A couple of comments:

• Choosing to change LED behavior only with SYSTEM_ERROR_NETWORK because the other errors don't necessarily lead to network failure. As per my understanding, network failure should be the one case that should get the LED back to green.

• The LED behavior also seem to largely depend on these states here such as these :

  device-os/system/src/system_network_manager.cpp

  Line 402 in 70f4f9a

  LED_SIGNAL_START(NETWORK_CONNECTED, BACKGROUND);

  There is an edge case where the device enters "Cloud Connecting" state, and fails DNS test, but it has not yet gotten a deregistration URC, in which case it is in IP_CONFIGURED state so the above code makes it breathe green, which is rightfully the correct state because it is in IP_CONFIGURED state. Once it gets the deregistration URC, it blinks green as expected. Please see this log as well. Just highlighting an edge case for reference (and it looks OK from my understanding).

ch64095.txt

IIRC, this issue is specific to Gen 3 platforms. On Gen 2, the system gets notified that it's no longer connected to the network and proceeds to update the LED (as well as the system state) accordingly via a call to cloud_disconnect(): https://github.com/particle-iot/device-os/blob/develop/system/src/system_network_internal.h#L677-L680.

Perhaps, rather than providing a workaround for just one specific case, where creating the cloud socket or "connecting" it to the server fails (note that UDP sockets are connection-less), we should investigate why the system doesn't update the cloud connection state accordingly when it's no longer connected to the network? I think we have a bigger problem here rather than just LED indication, and it probably dates back to mesh times when we, for some reason, weren't able to determine reliably which network interface among the available ones is used for the cloud connection.

cc @avtolstoy

@sergeuz

investigate why the system doesn't update the cloud connection state accordingly

Which state should the system update the cloud connectivity status to? Probably go back to "cloud disconnect" and restart the connection since we see a +CEREG: 2 URC?

+CEREG: 2 cellular deregistration* URC.

I discussed this PR with Andrey, and whereas it's possible that we're not cancelling the current ongoing connection attempt on network disconnection events in certain cases, trying to investigate and address that in the scope of this specific issue would be too much. The change introduced in this PR is perfectly fine: the establish_cloud_connection() function starts the LED indication and it should perform a cleanup and stop the indication if it wasn't able to establish a connection. I'm going to approve this PR 👍

okie thanks Sergey. Please let me know if we need to open a separate story for the broader correction.